-
Notifications
You must be signed in to change notification settings - Fork 825
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Routing logic to take both NamespaceID and workflowID into account #629
Routing logic to take both NamespaceID and workflowID into account #629
Conversation
Update the routing logic to use both NamespaceID and workflowID to compute the hash used for routing logic.
@@ -237,7 +237,10 @@ func (p *esProcessorImpl) hashFn(key interface{}) uint32 { | |||
return 0 | |||
} | |||
numOfShards := p.config.IndexerConcurrency() | |||
return uint32(common.WorkflowIDToHistoryShard(id, numOfShards)) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was no need to use WorkflowIDToHistoryShard as the hashing function. All this logic needed is a hashing function which takes in a string and computes range hashKey which is used to route the call to worker go routine processing ES messages.
@@ -403,7 +403,8 @@ message DescribeHistoryHostRequest { | |||
//ip:port | |||
string host_address = 1; | |||
int32 shard_id_for_host = 2; | |||
temporal.api.common.v1.WorkflowExecution execution_for_host = 3; | |||
string namespace_id = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why it is namespace
above and namespace_id
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They represent 2 different identifiers for namespace. namespace
is used whenever API takes namespace name and namespace_id
is used whenever API takes internal uuid which represents the namespace. Internally within the system we use namespace_id as the identifier.
hash := farm.Fingerprint32([]byte(workflowID)) | ||
// WorkflowIDToHistoryShard is used to map namespaceID-workflowID pair to a shardID | ||
func WorkflowIDToHistoryShard(namespaceID, workflowID string, numberOfShards int) int { | ||
idBytes := []byte(namespaceID + "_" + workflowID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does separator provides? We are hashing it right away.
What changed?
Update the routing logic to use both NamespaceID and workflowID to
compute the hash used for routing logic.
Why?
Cadence only uses
workflowID
as the key to compute the hash which is used for routing requests toshard hosting the workflow execution. This is a problem as multiple namespaces could use the same
workflowID and this logic could result in hot partition problems.
Updated the routing logic to use both namespaceID and workflowID to compute the hash key.
How did you test it?
unit and integration tests
Potential risks
This is a non backwards compatible change so this is why we are making it before our first production release.